feat(cloudflare): Capture request body via httpServerIntegration#20614
feat(cloudflare): Capture request body via httpServerIntegration#20614
Conversation
size-limit report 📦
|
JPeer264
force-pushed
the
jp/capture-request-body-cloudflare-workers
branch
2 times, most recently
from
ff11476 to
e0f8ddf
Compare
JPeer264
force-pushed
the
jp/capture-request-body-cloudflare-workers
branch
from
e0f8ddf to
0562008
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0562008. Configure here.
| const bodyPromise = clonedRequest.text(); | ||
| const timeoutPromise = new Promise<null>(resolve => { | ||
| setTimeout(() => resolve(null), 2000); | ||
| }); |
There was a problem hiding this comment.
Missing safeUnref on setTimeout in core package
Medium Severity
The setTimeout in captureBodyFromWinterCGRequest doesn't call safeUnref(), which could keep Node.js/Bun/Deno processes alive for up to 2 seconds after the main work completes. This is in @sentry/core, a server runtime package. The safeUnref utility exists in the same package at utils/timer.ts and is used elsewhere (e.g., promisebuffer.ts wraps setTimeout with safeUnref in the exact same Promise.race timeout pattern). The PR description notes this code is intended to be reusable for Bun and Deno, which support unref.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 0562008. Configure here.
| if (maxRequestBodySize === 'small') return 1_000; | ||
| if (maxRequestBodySize === 'medium') return 10_000; | ||
| return MAX_BODY_BYTE_LENGTH; | ||
| } |
There was a problem hiding this comment.
getMaxBodyByteLength returns 1MB for 'none' input
Medium Severity
getMaxBodyByteLength accepts the full MaxRequestBodySize type (including 'none') but doesn't handle 'none' explicitly — it falls through to return MAX_BODY_BYTE_LENGTH (1MB). This is the same value returned for 'always', making 'none' behave identically to "capture everything." The function is publicly exported from @sentry/core and the patchRequestToCaptureBody parameter type was also widened to accept MaxRequestBodySize, increasing the risk of a caller passing 'none' without an external guard.
Reviewed by Cursor Bugbot for commit 0562008. Configure here.
| if (client) { | ||
| await captureIncomingRequestBody(client, request); | ||
| } |
There was a problem hiding this comment.
m: Can be moved below the OPTIONS and HEAD check
| } | ||
|
|
||
| // Skip GET and HEAD requests - they don't have bodies | ||
| if (request.method === 'GET' || request.method === 'HEAD') { |
| if (maxRequestBodySize === 'small') return 1_000; | ||
| if (maxRequestBodySize === 'medium') return 10_000; | ||
| return MAX_BODY_BYTE_LENGTH; | ||
| } |
| export function getMaxBodyByteLength(maxRequestBodySize: MaxRequestBodySize): number { | ||
| if (maxRequestBodySize === 'small') return 1_000; | ||
| if (maxRequestBodySize === 'medium') return 10_000; | ||
| return MAX_BODY_BYTE_LENGTH; | ||
| } |
There was a problem hiding this comment.
We should likely handle 'none' in here too?
2 participants
closes #17079
closes JS-751
This PR is basically adding and exporting
captureBodyFromWinterCGRequestfrom@sentry/coreand re-implementinghttpServerIntegrationfor Cloudflare. There was no way to reuse the existinghttpServerIntegration, as it is usingpatchRequestToCaptureBody, which wouldn't work in Cloudflare, as there is too late to be patched, socaptureBodyFromWinterCGRequestwas born.The original
httpServerIntegrationis also taking care of other SDK processing metadata viahttpRequestToRequestData, which happens already for every request in Cloudflare via theaddHandlerin thescope-utils.ts. I still tried to reuse as much code as possible (that's the reason for the new exposed functionality in@sentry/core).Two options from the original integration are missing
sessionsandsessionFlushingDelayMS. I don't think this can be implemented 1:1, that is why I left it out, as the sessions seem to be aggregated, which wouldn't work in Cloudflare, as we generate a new client for each request. And I think it is better if we don't enable that by default, as otherwise every request would send a session by default.In theory this would also be usable for other runtimes like Bun or Deno if I'm not mistaken